Question is: where should we memoize it? Should we memoize it in the selector level, or component level? Or else where? 🤔

Hmmm, good question! 😅

I think it's reasonable to expect that getEntityRecords( ... ) !== getEntityRecords( ...) should only ever be true when there has been some change in state.

I played around in DevTools to check that this is already the case when there is data:

So, I think the fix should be to change this:

To use a shared reference to [] defined at the top of the file:

if ( ! queriedState ) {
	return EMPTY_ARRAY;
}

You can see that we've done similar optimisations in packages/editor/src/store/selectors.js.

@noisysocks That's actually my first solution!

However, the problem is that I feel like it's pretty normal to return a new instance of the data in the selectors. Take Array.prototype.map and Array.prototype.filter for example, they are commonly used in selectors to transform the state from the store, but they will also return a new instance every time being called. Moreover, I think it's not very clear and intuitive for developers to know when to memoize the value (using EMPTY_ARRAY technique) and when not to. In the end, I came to a conclusion that EMPTY_ARRAY feels more like a hack than an ultimate solution to me.

The other solution would be to memoize the selector using createSelector from rememo. That's normally how it's solved in Redux's world (reselect). However, this solution also has some problems. First of all, as developers, we still don't have any clue when to memoize the selectors. To me, memoizing selectors should be to fixing performance issues, not fixing referential equality issues or preventing bugs like infinite loops.

Secondly, rememo (and @wordpress/data in general) doesn't provide a way to memoize between different instances of the selector. getEntityRecords is used in several places/packages and they all have different arguments. AFAIK rememo only memoize the last call of the function (has cache size of 1). What that means is that repetitive calls of getEntityRecords in the same useSelect function might not be memoized if there's another call of getEntiryRecords in between from a different useSelect function.

1. getEntityRecords('postType', 'wp_block'): cache not hit, cached (1)
2. getEntityRecords('postType', 'post'): cache not hit, cleared (1) and cached (2)
3. getEntityRecords('postType', 'wp_block'): cache not hit, cleared (2) and cached (3)

Note that even though (3) and (1) have the same arguments, (3) still couldn't reuse the cache from (1) due to the cache size.

reselect offers a way to create selector creators so that different component can use different instances of the selector and prevent from sharing the same cache. @wordpress/data doesn't seem to provide an easy way to do this though 🤔 .

However, the problem is that I feel like it's pretty normal to return a new instance of the data in the selectors.

I don't think that's normal, we explicitly avoid this across our entire code base, in general when we "map", we always memoize the selector itself unless it's a selector that is used rarely. I also lean towards memoizing in the selectors (using the shared array here)

I think it's not very clear and intuitive for developers to know when to memoize the value (using EMPTY_ARRAY technique) and when not to.

I disagree, I feel it's more consistent to say that all selectors returning objects and arrays should be memoized rather than thinking adhoc in components.

Note that even though (3) and (1) have the same arguments, (3) still couldn't reuse the cache from (1) due to the cache size.

I'm not sure that's true, if it is, it's a bug, we explicitly designed our selectors to keep the cache for different arguments and not just limit it to 1. I see that getEntityRecords is memoized using equivalent-key-map which I believe works that way. If what you say is true, it's an important bug that need to be fixed.

I don't think that's normal, we explicitly avoid this across our entire code base, in general when we "map", we always memoize the selector itself unless it's a selector that is used rarely. I also lean towards memoizing in the selectors (using the shared array here)

I disagree, I feel it's more consistent to say that all selectors returning objects and arrays should be memoized rather than thinking adhoc in components.

If that's the case then it makes sense to me! I didn't know that there's such practice in our code base, and from a quick search I can still find several places which aren't memoizing selectors. Is this documented somewhere? I'm not sure if it counts as idiomatic code, but would help if it's documented explicitly.

we explicitly designed our selectors to keep the cache for different arguments and not just limit it to 1.
I don't think there's a bug. It turns out we have to explicitly use createSelector and list all the necessary dependencies in the second argument to make it work. Thanks for pointing this out!

I feel like using createSelector is a more general way of solving this, rather than, say, using EMPTY_ARRAY. I'll try to push a change to use createSelector instead, and maybe spend some time studying how createSelector really works 😅.

I feel like using createSelector is a more general way of solving this, rather than, say, using EMPTY_ARRAY. I'll try to push a change to use createSelector instead, and maybe spend some time studying how createSelector really works 😅.

true, but EMPTY_ARRAY is way less costfull.

I can still find several places which aren't memoizing selectors. Is this documented somewhere? I'm not sure if it counts as idiomatic code, but would help if it's documented explicitly.

We do have that document to explain some of the performance decisions but this one is definitely not there, it would be good to add.

https://developer.wordpress.org/block-editor/architecture/performance/

This Post also has some good context about the different performant-related work.

https://riad.blog/2020/02/14/a-journey-towards-a-performant-web-editor/

I wonder if we can automatically catch when selectors are incorrectly returning new instances by doing something like this in our unit tests.

import { getEntityRecords as _getEntityRecords } from '../selectors';

const checkSelectorStability = ( selector ) => ( ...args ) => {
	const firstResult = selector( ...args );
	const secondResult = selector( ...args );
	expect( secondResult ).toBe( firstResult );
	return secondResult;
};

const getEntityRecords = checkSelectorStability( _getEntityRecords );

Size Change: +14 B (0%)

Total Size: 1.21 MB

_{compressed-size-action}

I've added the Backport to Minor Release label so that if there's a 5.6.2 release this will be included.

If 5.6.2 doesn't happen and 5.7 is release the label can be removed, and it'll already be in 5.7.